Skip to content

Automated Test: topic-email-management #317

Conversation

admin-coderabbit
Copy link
Owner

@admin-coderabbit admin-coderabbit commented Feb 4, 2026

This pull request was automatically created by @coderabbitai/e2e-reviewer.

Batch created pull request.

Summary by CodeRabbit

Release Notes

  • New Features

    • Added ability to unsubscribe from topic notifications via dedicated unsubscribe page
    • Added one-click unsubscribe links directly in notification emails for easier notification management
  • Refactor

    • Modernized JavaScript syntax and improved code structure
    • Improved conditional rendering in dropdown components
  • Chores

    • Updated localization strings to support unsubscribe functionality

@coderabbit-eval
Copy link

coderabbit-eval bot commented Feb 4, 2026

📝 Walkthrough

Walkthrough

This pull request introduces a topic unsubscribe feature allowing users to stop receiving notifications for specific topics. It spans frontend UI components, backend controller logic, email integration, routing, and localization to enable toggling topic notification states.

Changes

Cohort / File(s) Summary
Frontend Components & Routes
app/assets/javascripts/discourse/components/dropdown-button.js.es6, app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6, app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6, app/assets/javascripts/discourse/routes/topic-from-params.js.es6, app/assets/javascripts/discourse/views/topic-unsubscribe.js.es6
New controller, route, and view for topic unsubscribe UI. Conditional title rendering in dropdown-button. Refactored topic-from-params to ES6 syntax with const-based scoping.
Frontend Routing & Templates
app/assets/javascripts/discourse/routes/app-route-map.js.es6, app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs, app/assets/stylesheets/common/base/topic.scss
Added topicUnsubscribe route mapping, new unsubscribe template with notification controls, and styling for layout/positioning of dropdown elements.
Backend Controller & Model
app/controllers/topics_controller.rb, app/models/topic.rb
New unsubscribe action toggling topic notification levels. New unsubscribe_url method on Topic model. Updated before filters to include unsubscribe action.
Email Integration
app/mailers/user_notifications.rb, lib/email/message_builder.rb, app/views/email/notification.html.erb
Added unsubscribe_url to email context. Updated email template footer with unsubscribe link. Minor message builder formatting.
Routing & Configuration
config/routes.rb
Added unsubscribe endpoints for both slug-based and numeric topic IDs. Added summary routes with appropriate constraints and defaults.
Localization
config/locales/client.en.yml, config/locales/server.en.yml
New topic.unsubscribe translation keys for UI. Extended unsubscribe_link with topic-specific messaging for emails.
Data Model & Tests
app/models/topic_user.rb, spec/components/email/message_builder_spec.rb
Whitespace/formatting adjustments in topic_user.rb. Added unsubscribe_url parameter to message builder test constructor.

Sequence Diagram

sequenceDiagram
    actor User
    participant Client as Client (Browser)
    participant Route as Topic Unsubscribe Route
    participant Controller as TopicsController
    participant Topic as Topic Model
    participant DB as Database
    participant Notifier as Notification System

    User->>Client: Click unsubscribe link
    Client->>Route: GET /t/:slug/:id/unsubscribe
    Route->>Route: Load topic via PostStream.loadTopicView
    Route->>Controller: Route to unsubscribe action
    Controller->>Topic: Create/retrieve topic
    Topic->>DB: Fetch topic data
    DB-->>Topic: Topic record
    Controller->>DB: Update TopicUser notification_level<br/>(toggle muted/regular)
    DB-->>Controller: Confirmation
    Controller->>Controller: Build TopicView
    Controller->>Client: Render show template with updated state
    Client-->>User: Display unsubscribe confirmation
    
    Note over Notifier: Future notifications<br/>will respect new<br/>notification_level
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 A whisker-twitch of topic care,
No more alerts to fill the air!
With one small click, the noise does cease,
Notifications fade to peace.
Unsubscribe, a simple plea,
For quieter burrows, wild and free! 🌿

🚥 Pre-merge checks | ✅ 1 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Automated Test: topic-email-management' is generic and vague, using non-descriptive phrasing that doesn't convey the actual changes made to the codebase. Provide a specific, descriptive title that summarizes the main change—e.g., 'Add topic unsubscribe feature with email notifications management' to clearly indicate the primary functionality added.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch topic-email-management

Comment @coderabbit eval help to get the list of available commands and usage tips.

@coderabbit-eval coderabbit-eval bot closed this Feb 4, 2026
Copy link

@coderabbit-eval coderabbit-eval bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
spec/components/email/message_builder_spec.rb (1)

167-180: ⚠️ Potential issue | 🟡 Minor

Missing test coverage for the new unsubscribe_url parameter.

The unsubscribe_url is passed to MessageBuilder on line 172, but no assertion verifies it is actually used — e.g., that it appears in the rendered body or in a header. The existing specs only check List-Unsubscribe presence and user_preferences_url in body.

Consider adding a test like:

it "has the unsubscribe url in the body" do
  expect(message_with_unsubscribe.body).to match("/t/1234/unsubscribe")
end
🤖 Fix all issues with AI agents
In `@app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6`:
- Around line 5-7: Rename the misspelled controller property
stopNotificiationsText to stopNotificationsText in topic-unsubscribe.js.es6
(preserve the .property("model.fancyTitle") observer) and update the
corresponding reference in unsubscribe.hbs (and any other code/tests referencing
stopNotificiationsText) to the corrected stopNotificationsText so bindings and
templates continue to work.

In `@app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs`:
- Around line 1-7: Rename the misspelled property stopNotificiationsText to
stopNotificationsText everywhere it’s defined and referenced: update the
template usage ({{{stopNotificiationsText}}}) and change the corresponding
property in the controller/component that provides it (where
stopNotificiationsText is declared/computed), then run tests/grep to update any
other occurrences (helpers, tests, or usages like bindings) to the corrected
stopNotificationsText to keep behavior identical but fix the typo.

In `@app/controllers/topics_controller.rb`:
- Around line 107-111: The current GET handler mutates
TopicUser.notification_level (tu.notification_level) by toggling between
notification_levels[:regular] and [:muted]; change it so GET no longer mutates
state: have the controller render a confirmation page on GET (showing the
pending unsubscribe) and perform the actual update in a separate POST action
(e.g., unsubscribe_confirm or the same action constrained to POST). Also change
the mutation logic to be one-way: when performing the POST update, set
tu.notification_level = TopicUser.notification_levels[:muted] (do not toggle)
and persist the change; keep references to the existing tu variable and
TopicUser.notification_levels to locate the code.
- Around line 98-116: In unsubscribe, TopicUser.find_by may return nil causing a
NoMethodError on tu; update the unsubscribe action to handle a missing
TopicUser: fetch with TopicUser.find_or_initialize_by(user_id: current_user.id,
topic_id: params[:topic_id]) (or explicitly return/redirect if you prefer), then
set tu.notification_level using the same conditional logic and call tu.save!;
keep the rest of the flow (perform_show_response and redirect logic) unchanged
and reference the tu/local variable, TopicUser.find_by/find_or_initialize_by,
and the unsubscribe method.

In `@app/views/email/notification.html.erb`:
- Line 10: The class attribute on the H4 element is using a CSS selector dot
(class='.previous-discussion') which results in a literal dot in the HTML class
and prevents selectors like .previous-discussion from matching; edit the H4 tag
in notification.html.erb (the element with <%= t
"user_notifications.previous_discussion" %>) to remove the leading dot so the
class is previous-discussion, and update any CSS if it was accidentally written
to target the dotted literal class.
- Around line 5-20: The respond_instructions footer is rendered twice when
context_posts.present? is true; remove the duplicated instance inside the
conditional block so only the unconditional "<div
class='footer'>%{respond_instructions}</div>" remains at the bottom, leaving the
context_posts block (and its render partial: 'email/post') intact and unchanged.

In `@config/routes.rb`:
- Line 439: The slug-less summary route (get "t/:topic_id/summary") is missing
defaults: {summary: true}, so params[:summary] won't be set when hitting
topics#show; update that route to include defaults: {summary: true} like the
slugged route (get "t/:slug/:topic_id/summary" => "topics#show", defaults:
{summary: true}, constraints: {topic_id: /\d+/}) so both routes set
params[:summary] for TopicsController#show.
- Around line 440-441: The unsubscribe route currently maps GET
"t/:slug/:topic_id/unsubscribe" and GET "t/:topic_id/unsubscribe" to
topics#unsubscribe which mutates state (calls tu.save!) and is unsafe; change
the routes to use a non-idempotent verb (POST or DELETE) for the state-changing
endpoint and/or implement a GET confirmation flow: keep a GET action (e.g.,
topics#unsubscribe_confirm) that renders a confirmation page and move the actual
mutation into a separate POST/DELETE action (e.g., topics#unsubscribe or
topics#destroy_subscription) which performs the tu.save! logic; update the
routes to point GET -> topics#unsubscribe_confirm and POST/DELETE ->
topics#unsubscribe (or new name), and adjust the controller
(topics_controller.rb) to split the confirmation and mutation accordingly and
guard the mutation behind the non-GET verb.
🧹 Nitpick comments (6)
config/locales/client.en.yml (1)

984-986: New unsubscribe translation keys look appropriate.

Minor: change_notification_state is missing a trailing period, while stop_notifications ends with one. Consider making punctuation consistent across these sibling keys.

lib/email/message_builder.rb (1)

62-62: Wrap assignment-in-condition in parentheses to satisfy RuboCop.

RuboCop flags if response_instructions = ... as ambiguous (assignment vs. comparison). Wrapping in parentheses makes the intent explicit.

✏️ Proposed fix
-        if response_instructions = `@template_args`[:respond_instructions]
+        if (response_instructions = `@template_args`[:respond_instructions])
app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6 (2)

4-9: loadTopicView fetches the full topic (including posts) just for the unsubscribe page.

This loads significantly more data than needed. The unsubscribe UI only requires the topic title and notification state. Consider using a lighter endpoint if one exists, or adding one, to reduce payload and latency for this page.


4-9: No error handling for failed topic load.

If the topic doesn't exist or the user lacks access, the loadTopicView promise rejects with no explicit handling. Ember will transition to the error route by default, but a user-friendly message (e.g., "topic not found or you don't have access") would improve the experience.

app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6 (1)

1-3: Consider migrating to Ember.Controller to future-proof the code.

ObjectController is deprecated in Ember and should not be used in new code. This codebase already provides a modern alternative via discourse/controllers/controller, which wraps Ember.Controller with the same Presence mixin. Update the import from discourse/controllers/object to discourse/controllers/controller. This pattern is used consistently across multiple controllers in the codebase and eliminates reliance on deprecated Ember APIs.

app/controllers/topics_controller.rb (1)

27-28: Nit: trailing comma after last array element.

RuboCop flags a missing trailing comma. Adding one is consistent with the project's style and makes future diffs cleaner.

Suggested fix
-                                          :bookmark,
-                                          :unsubscribe]
+                                          :bookmark,
+                                          :unsubscribe,]

Comment on lines +5 to +7
stopNotificiationsText: function() {
return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") });
}.property("model.fancyTitle"),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Typo: stopNotificiationsTextstopNotificationsText

Same typo flagged in the template — extra 'i' in "Notificiations". Fix here and in unsubscribe.hbs together.

✏️ Proposed fix
-  stopNotificiationsText: function() {
+  stopNotificationsText: function() {
     return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") });
   }.property("model.fancyTitle"),
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
stopNotificiationsText: function() {
return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") });
}.property("model.fancyTitle"),
stopNotificationsText: function() {
return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") });
}.property("model.fancyTitle"),
🤖 Prompt for AI Agents
In `@app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6` around
lines 5 - 7, Rename the misspelled controller property stopNotificiationsText to
stopNotificationsText in topic-unsubscribe.js.es6 (preserve the
.property("model.fancyTitle") observer) and update the corresponding reference
in unsubscribe.hbs (and any other code/tests referencing stopNotificiationsText)
to the corrected stopNotificationsText so bindings and templates continue to
work.

Comment on lines +1 to +7
<div class="container">
<p>
{{{stopNotificiationsText}}}
</p>
<p>
{{i18n "topic.unsubscribe.change_notification_state"}} {{topic-notifications-button topic=model}}
</p>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Typo in property name: stopNotificiationsTextstopNotificationsText

The property name has an extra 'i' — Notificiations instead of Notifications. While this is consistent between the template and controller (so it works), it will be confusing for future maintainers and should be corrected in both places.

✏️ Proposed fix
 <div class="container">
   <p>
-    {{{stopNotificiationsText}}}
+    {{{stopNotificationsText}}}
   </p>
   <p>
     {{i18n "topic.unsubscribe.change_notification_state"}} {{topic-notifications-button topic=model}}
   </p>
 </div>
🤖 Prompt for AI Agents
In `@app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs` around
lines 1 - 7, Rename the misspelled property stopNotificiationsText to
stopNotificationsText everywhere it’s defined and referenced: update the
template usage ({{{stopNotificiationsText}}}) and change the corresponding
property in the controller/component that provides it (where
stopNotificiationsText is declared/computed), then run tests/grep to update any
other occurrences (helpers, tests, or usages like bindings) to the corrected
stopNotificationsText to keep behavior identical but fix the typo.

Comment on lines +98 to +116
def unsubscribe
@topic_view = TopicView.new(params[:topic_id], current_user)

if slugs_do_not_match || (!request.format.json? && params[:slug].blank?)
return redirect_to @topic_view.topic.unsubscribe_url, status: 301
end

tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id])

if tu.notification_level > TopicUser.notification_levels[:regular]
tu.notification_level = TopicUser.notification_levels[:regular]
else
tu.notification_level = TopicUser.notification_levels[:muted]
end

tu.save!

perform_show_response
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

TopicUser.find_by can return nil, causing NoMethodError on line 107.

If the current user has never interacted with the topic (no TopicUser record exists), find_by returns nil, and tu.notification_level on line 107 will raise NoMethodError. This is a realistic scenario — a user may receive an email notification but have no TopicUser row yet.

🐛 Proposed fix
-    tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id])
-
-    if tu.notification_level > TopicUser.notification_levels[:regular]
-      tu.notification_level = TopicUser.notification_levels[:regular]
-    else
-      tu.notification_level = TopicUser.notification_levels[:muted]
-    end
-
-    tu.save!
+    tu = TopicUser.find_or_initialize_by(user_id: current_user.id, topic_id: params[:topic_id])
+    tu.notification_level = TopicUser.notification_levels[:muted]
+    tu.save!
🧰 Tools
🪛 RuboCop (1.84.0)

[convention] 98-116: Assignment Branch Condition size for unsubscribe is too high. [<4, 31, 6> 31.83/23]

(Metrics/AbcSize)

🤖 Prompt for AI Agents
In `@app/controllers/topics_controller.rb` around lines 98 - 116, In unsubscribe,
TopicUser.find_by may return nil causing a NoMethodError on tu; update the
unsubscribe action to handle a missing TopicUser: fetch with
TopicUser.find_or_initialize_by(user_id: current_user.id, topic_id:
params[:topic_id]) (or explicitly return/redirect if you prefer), then set
tu.notification_level using the same conditional logic and call tu.save!; keep
the rest of the flow (perform_show_response and redirect logic) unchanged and
reference the tu/local variable, TopicUser.find_by/find_or_initialize_by, and
the unsubscribe method.

Comment on lines +107 to +111
if tu.notification_level > TopicUser.notification_levels[:regular]
tu.notification_level = TopicUser.notification_levels[:regular]
else
tu.notification_level = TopicUser.notification_levels[:muted]
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Toggle semantics on a GET "unsubscribe" action are problematic.

Two concerns:

  1. GET mutating state: This action is routed via GET (for email link clickability), but it modifies TopicUser notification level. This violates HTTP semantics and can be triggered by prefetch/preload mechanisms in email clients, browsers, or link scanners. Consider rendering a confirmation page on GET and performing the actual mutation on POST.

  2. Toggle is unintuitive for "unsubscribe": Users clicking an "unsubscribe" link expect a one-way action (stop notifications). The current toggle means clicking the link a second time would re-subscribe, which is unexpected. Consider always setting the level to :muted (or :regular at minimum) instead of toggling.

🤖 Prompt for AI Agents
In `@app/controllers/topics_controller.rb` around lines 107 - 111, The current GET
handler mutates TopicUser.notification_level (tu.notification_level) by toggling
between notification_levels[:regular] and [:muted]; change it so GET no longer
mutates state: have the controller render a confirmation page on GET (showing
the pending unsubscribe) and perform the actual update in a separate POST action
(e.g., unsubscribe_confirm or the same action constrained to POST). Also change
the mutation logic to be one-way: when performing the POST update, set
tu.notification_level = TopicUser.notification_levels[:muted] (do not toggle)
and persist the change; keep references to the existing tu variable and
TopicUser.notification_levels to locate the code.

Comment on lines +5 to +20
<% if context_posts.present? %>
<div class='footer'>%{respond_instructions}</div>

<hr>

<% context_posts.each do |p| %>
<%= render :partial => 'email/post', :locals => {:post => p} %>
<h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>

<% context_posts.each do |p| %>
<%= render partial: 'email/post', locals: { post: p } %>
<% end %>
<% end %>
<% end %>

<hr>
<hr>

<div class='footer'>%{respond_instructions}</div>
<div class='footer'>%{unsubscribe_link}</div>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

%{respond_instructions} is rendered twice when context_posts are present.

Line 6 outputs %{respond_instructions} inside the if context_posts.present? block, and line 19 outputs it again unconditionally. When context posts exist, the respond instructions will appear twice in the email.

If the intent is to show respond instructions between the main post and the context posts (as a separator), and then again at the bottom, this is likely unintentional duplication. Otherwise, remove one occurrence.

🐛 Suggested fix — keep only the unconditional footer
   <% if context_posts.present? %>
-    <div class='footer'>%{respond_instructions}</div>
-
     <hr>

     <h4 class='previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>

     <% context_posts.each do |p| %>
       <%= render partial: 'email/post', locals: { post: p } %>
     <% end %>
   <% end %>

   <hr>

   <div class='footer'>%{respond_instructions}</div>
   <div class='footer'>%{unsubscribe_link}</div>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<% if context_posts.present? %>
<div class='footer'>%{respond_instructions}</div>
<hr>
<% context_posts.each do |p| %>
<%= render :partial => 'email/post', :locals => {:post => p} %>
<h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>
<% context_posts.each do |p| %>
<%= render partial: 'email/post', locals: { post: p } %>
<% end %>
<% end %>
<% end %>
<hr>
<hr>
<div class='footer'>%{respond_instructions}</div>
<div class='footer'>%{unsubscribe_link}</div>
<% if context_posts.present? %>
<hr>
<h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>
<% context_posts.each do |p| %>
<%= render partial: 'email/post', locals: { post: p } %>
<% end %>
<% end %>
<hr>
<div class='footer'>%{respond_instructions}</div>
<div class='footer'>%{unsubscribe_link}</div>
🤖 Prompt for AI Agents
In `@app/views/email/notification.html.erb` around lines 5 - 20, The
respond_instructions footer is rendered twice when context_posts.present? is
true; remove the duplicated instance inside the conditional block so only the
unconditional "<div class='footer'>%{respond_instructions}</div>" remains at the
bottom, leaving the context_posts block (and its render partial: 'email/post')
intact and unchanged.


<% context_posts.each do |p| %>
<%= render :partial => 'email/post', :locals => {:post => p} %>
<h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

CSS selector dot in HTML class attribute.

class='.previous-discussion' includes a leading dot, which is CSS selector syntax and not valid as an HTML class name. The dot will be literally included in the class, so any CSS rule targeting .previous-discussion won't match.

🐛 Proposed fix
-    <h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>
+    <h4 class='previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>
<h4 class='previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>
🤖 Prompt for AI Agents
In `@app/views/email/notification.html.erb` at line 10, The class attribute on the
H4 element is using a CSS selector dot (class='.previous-discussion') which
results in a literal dot in the HTML class and prevents selectors like
.previous-discussion from matching; edit the H4 tag in notification.html.erb
(the element with <%= t "user_notifications.previous_discussion" %>) to remove
the leading dot so the class is previous-discussion, and update any CSS if it
was accidentally written to target the dotted literal class.

get "t/:slug/:topic_id/summary" => "topics#show", defaults: {summary: true}, constraints: {topic_id: /\d+/, post_number: /\d+/}
get "t/:topic_id/summary" => "topics#show", constraints: {topic_id: /\d+/, post_number: /\d+/}
get "t/:slug/:topic_id/moderator-liked" => "topics#moderator_liked", constraints: {topic_id: /\d+/}
get "t/:slug/:topic_id/summary" => "topics#show", defaults: {summary: true}, constraints: {topic_id: /\d+/}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Missing defaults: {summary: true} on the slug-less summary route.

Line 439 sets defaults: {summary: true} for t/:slug/:topic_id/summary, but line 442 (t/:topic_id/summary) omits it. Both routes point to topics#show, so the slug-less variant will not set params[:summary] to true, likely breaking the summary view when accessed without a slug.

🐛 Proposed fix
-  get "t/:topic_id/summary" => "topics#show", constraints: {topic_id: /\d+/}
+  get "t/:topic_id/summary" => "topics#show", defaults: {summary: true}, constraints: {topic_id: /\d+/}

Also applies to: 442-442

🤖 Prompt for AI Agents
In `@config/routes.rb` at line 439, The slug-less summary route (get
"t/:topic_id/summary") is missing defaults: {summary: true}, so params[:summary]
won't be set when hitting topics#show; update that route to include defaults:
{summary: true} like the slugged route (get "t/:slug/:topic_id/summary" =>
"topics#show", defaults: {summary: true}, constraints: {topic_id: /\d+/}) so
both routes set params[:summary] for TopicsController#show.

Comment on lines +440 to +441
get "t/:slug/:topic_id/unsubscribe" => "topics#unsubscribe", constraints: {topic_id: /\d+/}
get "t/:topic_id/unsubscribe" => "topics#unsubscribe", constraints: {topic_id: /\d+/}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, find the topics controller file
find . -type f -name "*topics*controller*"

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 271


🏁 Script executed:

# Check the unsubscribe action in the topics controller
ast-grep --pattern $'def unsubscribe
  $$$
end'

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 2521


Refactor unsubscribe routes to use POST/DELETE or implement a GET confirmation flow.

The topics#unsubscribe action on lines 98-116 of app/controllers/topics_controller.rb directly mutates state by modifying the user's notification level and calling tu.save! on GET requests. This creates a serious security and usability issue: email link scanners, browser prefetching, and web crawlers can trigger the unsubscribe without user intent.

Consider either:

  1. Change the routes to use POST/DELETE instead of GET
  2. Or keep GET for a confirmation page but move the actual state mutation to a separate POST/DELETE action
🤖 Prompt for AI Agents
In `@config/routes.rb` around lines 440 - 441, The unsubscribe route currently
maps GET "t/:slug/:topic_id/unsubscribe" and GET "t/:topic_id/unsubscribe" to
topics#unsubscribe which mutates state (calls tu.save!) and is unsafe; change
the routes to use a non-idempotent verb (POST or DELETE) for the state-changing
endpoint and/or implement a GET confirmation flow: keep a GET action (e.g.,
topics#unsubscribe_confirm) that renders a confirmation page and move the actual
mutation into a separate POST/DELETE action (e.g., topics#unsubscribe or
topics#destroy_subscription) which performs the tu.save! logic; update the
routes to point GET -> topics#unsubscribe_confirm and POST/DELETE ->
topics#unsubscribe (or new name), and adjust the controller
(topics_controller.rb) to split the confirmation and mutation accordingly and
guard the mutation behind the non-GET verb.

Sign in to join this conversation on GitHub.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant